-
Notifications
You must be signed in to change notification settings - Fork 392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#6302] refactor:Optimize Flink connector properties converter #6303
base: main
Are you sure you want to change the base?
Conversation
aecdd3a
to
03c0d52
Compare
fixed ci error : #6305 |
03c0d52
to
97bc2a7
Compare
@FANNG1 PTAL , thanks |
...ink/src/main/java/org/apache/gravitino/flink/connector/paimon/PaimonPropertiesConverter.java
Outdated
Show resolved
Hide resolved
...-connector/flink/src/main/java/org/apache/gravitino/flink/connector/PropertiesConverter.java
Outdated
Show resolved
Hide resolved
f576a21
to
92d0edb
Compare
@FANNG1 Fixed all , PTAL again, thanks |
...-connector/flink/src/main/java/org/apache/gravitino/flink/connector/PropertiesConverter.java
Outdated
Show resolved
Hide resolved
...-connector/flink/src/main/java/org/apache/gravitino/flink/connector/PropertiesConverter.java
Show resolved
Hide resolved
...c/test/java/org/apache/gravitino/flink/connector/iceberg/TestIcebergPropertiesConverter.java
Show resolved
Hide resolved
9668495
to
dbeb10b
Compare
dbeb10b
to
364591c
Compare
@@ -50,7 +52,11 @@ void testCatalogPropertiesWithHiveBackend() { | |||
IcebergPropertiesConstants.ICEBERG_CATALOG_URI, | |||
"hive-uri", | |||
IcebergPropertiesConstants.ICEBERG_CATALOG_WAREHOUSE, | |||
"hive-warehouse"), | |||
"hive-warehouse", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key1 and value1 should not exists in the result. By design only the keys defined by Graviitno like catalog-backend
or start with flink.bypass.
are passed to the underlying flink connector. the other property keys are dropped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current unified logic is to automatically add a prefix to parameters that have not been mapped and have not been added with "Flink. bypass." before passing them to Gravitino. When setting them, there is no mandatory restriction that they must include "Flink. bypass, This is also for compatibility with old logic, but what you said is also reasonable. Let me unify it first
@@ -65,7 +71,7 @@ void testCatalogPropertiesWithRestBackend() { | |||
"rest-uri", | |||
IcebergPropertiesConstants.GRAVITINO_ICEBERG_CATALOG_WAREHOUSE, | |||
"rest-warehouse", | |||
"key1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you use key1
and flink.bypass.key2
too?
LGTM except minor comments |
What changes were proposed in this pull request?
Why are the changes needed?
Fix: #(6302)
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
N/A